feat(mcp): add manage_adr mode='append' - #1243

CI is green — this closes the verification gap flagged in the description

The PR description says compilation was unverified because no C toolchain was available where this was written. CI has now covered exactly that, so please disregard that caveat: all 25 checks pass.

Builds and full suites on every target: pr-smoke (ubuntu-latest, macos-14, windows-latest), test-unix (ubuntu-latest + ubuntu-24.04-arm with gcc/g++, macos-14 + macos-15-intel with cc/c++), test-windows (CLANG64/x86_64), test-tsan (ubuntu-latest, ubuntu-24.04-arm, macos-14). Plus lint, security / security-static, security / codeql-gate, security / license-gate, dco, test / test-windows-guards, test / shard-completeness.

The four new tests actually executed

"Suite passed" is not the same claim as "my tests ran", so I checked. Per-test names are not in the CI logs — a control grep for the pre-existing tool_manage_adr_unified_backend_issue256 also returns nothing, so their absence proves nothing either way. The test counts do prove it. Comparing this branch against the main run at b6b55192:

tests/test_mcp.c runs in shard 1/3, which is up by exactly the four added cases, with 0 failed throughout. No other shard moved, so nothing else was disturbed.

One note on the audit, in case it comes up in review: scripts/security-audit.sh passes but prints REVIEW: src/mcp/mcp.c has 17 file read operations (expected max 15). That is pre-existing drift against EXPECTED_MAX, not something this PR introduces — git show HEAD~1:src/mcp/mcp.c | grep -c 'fopen\|fread\|read_file' is also 17. This change adds no file reads. Happy to bump the constant in a separate commit if you'd like it brought back in line, but I left it alone to keep this PR scoped.

Thanks for the careful tests and docs. This needs an ADR-model decision before implementation approval. The current append mode is a non-idempotent read-concatenate-replace operation in the MCP handler, so a retry after a lost response duplicates content; it also bypasses the existing store-level section-update path and its 8,000-character bound. Leading newlines in the addition violate the advertised one-blank-line rule, and empty content can report success while appending zero bytes. If append semantics are accepted, the implementation will need store-layer atomicity, explicit retry/idempotency and size rules, plus tests for repeated requests, concurrent append, leading newlines, empty content, and response length fields. No need to revise until the maintainer direction is decided.

Thanks for this, and sorry for the wait. Queued for review.

MERGEABLE with all 28 checks green, +205/-3 across three files.

An append mode for manage_adr is a sensible-sounding addition — ADR content is naturally accumulative and a read-modify-write from the client side is easy to get wrong. It does extend the MCP tool contract, so it will get a look on interface design as well as implementation.

Reviewed in full. The implementation is clean and the security question came back clean too — what is holding this is a surface decision that belongs to the maintainer, not a problem with your work.

The security question I most wanted answered, answered. Because this mode writes content at an LLM's request, I went looking for a path-traversal surface. There isn't one: the append path performs no filesystem write at all. Storage is the SQLite project_summaries table through a parameterised UPSERT with project and content as bound parameters, so no path is ever constructed from tool arguments — a hostile project value is simply an unmatched database key and the handler errors out before any write. No raw fopen added anywhere; the only nearby file read is the pre-existing legacy-migration path, which builds from the store-registered root and already uses cbm_fopen(). The UPSERT is atomic, so an interrupted process leaves the old row or the new one, never a half-written document.

Things you got right that are worth naming:

• The missing_content guard is a genuinely good catch. Without it, append with no content would fall through to the get branch and hand a writer a success-shaped read. You spotted a real pre-existing trap pattern and closed it — and you deliberately left the analogous update fall-through out of scope with a note, which is exactly the right scoping instinct.
• The tests are better than most. Byte-precise ASSERT_STR_EQ on the exact stored document, the single-blank-line contract asserted as literal bytes, and — the detail I liked most — the no-content case verifies the stored ADR is byte-identical afterwards, not merely that an error came back. All four fail without the change.
• The whitespace policy (trim trailing \n/\r, exactly one blank line, degrade-to-create on an empty or missing ADR) is carefully reasoned, and the CRLF handling shows you were thinking about the Windows story this project cares about.
• Your PR description was honest that you could not compile locally, and explained what you did instead — checking every API against existing call sites and porting the join logic to a scratch harness. CI has since confirmed green across all platforms and the sanitizer legs, so that caveat is resolved.

Why it is not merged yet. Adding a mode to manage_adr is permanent public MCP surface — a one-way door — and the same bytes are reachable today via get, client-side concatenation, and update. Your counter-argument is a good one and I have passed it on rather than paraphrasing it away: with update the agent must re-emit the whole document, so every unchanged byte survives only as well as the LLM round-trip, whereas server-side concatenation makes the stored copy the authority for the prefix. That is a data-integrity argument, not a token-saving one, and it is the strongest thing in favour.

One alternative the maintainer will weigh, which you could not have known about: the store layer already contains an unexposed cbm_store_adr_update_sections (store.c:7381) with zero callers outside store.c, and it already enforces the 8000-character CBM_ADR_MAX_LENGTH cap. A section-level update mode may end up being the shape we open instead.

Three things worth fixing regardless of which way that lands:

1. No size cap on append. CBM_ADR_MAX_LENGTH is enforced only by that unexposed section function — MCP update and now append both bypass it. Append in particular makes unbounded incremental growth cheap, and each call is an O(n) full re-read and re-write, after which an oversized ADR bloats every get_architecture response. Either reject above a cap (status='too_large'), or we decide deliberately that ADRs are uncapped and delete the dead constant.
2. A lost-update race that is new. get → merge → store are separate statements rather than a transaction. update today is a single atomic UPSERT, so this read-modify-write window does not currently exist — append introduces it. A concurrent writer (the UI /api/adr full replace, or a second MCP client) landing between the read and the UPSERT gets silently overwritten by stale-prefix-plus-addition. Low severity for a local single-user tool, and BEGIN IMMEDIATE … COMMIT around the pair closes it.
3. Minor: content: "" passes the !content check and rewrites the row while appending nothing — it probably deserves missing_content too.

I will come back to you with the direction answer. Thank you for the care on this one; the review was quick because the work was tidy.

The direction answer I promised on 31 July, a month late — I am sorry it took this long, and doubly so because you were told to hold off and did.

The decision: the ADR write surface grows a section-level update mode, built on cbm_store_adr_update_sections — not whole-document append. I want to lay out the reasoning fully, because your integrity argument was correct and deserves a real answer rather than a verdict.

You identified the true problem: with update, the unchanged prefix survives only as well as the LLM round-trip that re-emits it. Append solves that — but it carries one property I could not get past: it is non-idempotent on client retry. An MCP client that loses a response and retries duplicates the chunk, silently. Every mitigation (client tokens, content hashing) adds surface to fix a problem the sectioned shape does not have: setting a section twice yields the same state. Retry-safety by construction beats retry-safety by protocol.

The section shape also happens to be cap-enforced already (CBM_ADR_MAX_LENGTH is applied in cbm_store_adr_update_sections, which closes the unbounded-growth issue from my review for free), and "append an entry" maps naturally onto "add a new section" — the ADR is a sectioned document, and entries with headings is how every long-lived one I have seen actually grows.

The offer: this lands as your contribution if you want it. The store layer is built and tested; what remains is exposing it through manage_adr — and the ground has moved in a way I owe you a map of:

• ADR writes now take a per-project mutation lease and a dedicated writable store (write_request at mcp.c:11193, open_adr_store_for_write at :11143); the new mode joins that classification or it bypasses the protection main added.
• The cross-process race from my review still wants BEGIN IMMEDIATE around read-modify-write.
• The content:"" guard and the empty/missing-ADR degrade behaviour from your append design carry over almost verbatim.
• Your four tests translate directly — set-new-section, set-existing-section, and the two error shapes — plus the retry case that motivated everything: set the same section twice, assert byte-identical state.

If the month has moved you on, say the word and I will implement it with Co-Authored-By: credit — the analysis that got us here is yours either way. And thank you for the most rigorous CI-green comment this queue has seen; proving your tests executed via per-shard pass-count deltas is a habit I wish more of us had.

@andis777 — taking the second half of my own offer rather than leaving you waiting: I'm implementing the section-level mode in-house, with Co-authored-by: credit to you. You'll get the landing SHA here when it merges.

To be straight about why, since I offered you first refusal barely a day ago and am not waiting for the answer: the queue is moving now, the branch has drifted hard (80 commits have touched src/mcp/mcp.c since yours, and handle_manage_adr was rewritten by fb7a6358 — the very commit that added the lease machinery the new mode needs), and asking you to absorb that after a month of waiting on my direction answer isn't a fair trade. If you'd rather have carried it, say so and I'll hand it back.

Your work loses on exactly one property, and it isn't quality. Append is non-idempotent on client retry — a lost response means a silently duplicated chunk. The sectioned shape is retry-safe by construction and already cap-enforced. Everything else in your PR held up under review: the implementation was clean, the string handling correct and bounds-checked, no filesystem exposure at all, and your scope discipline was exemplary — you declined to fix the analogous update fall-through and declined to bump the audit constant, both with notes explaining why. That is unusual and I noticed.

Your CI comment was the best evidence in this entire queue. Proving your four tests actually executed via per-shard pass-count deltas (1854 → 1858, other shards unchanged), and then explicitly noting that a control grep for an existing test name also returns nothing so log-absence proves nothing either way — that is precisely the right discipline. Three separate agents have been caught out by "wrong-suite false greens" in this repo in the last day; you pre-empted it unprompted on your first contribution.

Two defects review turned up that neither of us had named, recorded here because they'd have bitten whoever implemented this:

1. The append branch would have run through a query-only store handle with no mutation lease — write_request at mcp.c:11214 lists only update/store, and it gates the lease, the query-only flag and the write-store open. Any new write mode must be added there.
2. It read have_adr ? adr.content : NULL, dropping main's legacy fallback at :11321 — so a legacy ADR whose best-effort migration hadn't taken would have been silently discarded.

Both are carried into the new implementation, along with your empty-content and degrade-to-create guards, and a BEGIN IMMEDIATE around the read-modify-write — there turn out to be three full-replace writers of that row, including the indexing pipeline itself.

Thank you for the patience and for a genuinely strong first contribution.

Landed. manage_adr mode='set_sections' is on main as of 997d087b (PR #1904), with Co-authored-by: credit to you.

@andis777 — thank you, and a straight account of what happened to your work.

Your implementation was clean; it lost on one property. Whole-document append is non-idempotent on client retry — a lost response means a silently duplicated chunk. Section updates are retry-safe by construction. That was the only thing wrong with it, and everything else in your PR held up: correct bounds-checked string handling, no filesystem exposure at all (parameterised UPSERT, no path ever built from tool arguments), and genuinely exemplary scope discipline — you declined to fix the analogous update fall-through and declined to bump the audit constant, both with notes explaining why.

Your CI comment was the best evidence in this queue. Proving your four tests actually executed via per-shard pass-count deltas (1854 → 1858, other shards unchanged), then noting that a control grep for an existing test name also returns nothing so log-absence proves nothing either way — that is exactly the right discipline. Several agents working this repo have been caught by "wrong-suite false greens" in the last two days; you pre-empted it unprompted on a first contribution.

Two defects your review turned up that nobody had named, both now fixed and both of which would have bitten whoever implemented this:

1. A new write mode absent from the write_request classification runs through a query-only store handle with no mutation lease — that one boolean gates the lease, the query-only flag and the write-store open.
2. The legacy fallback was being dropped, so a legacy ADR whose best-effort migration had not taken would have been silently discarded. Interestingly the naive fix does not work either: adding the mode to write_request skips the migration block that populates legacy, so the write path now reads the legacy file itself under the lease it already holds.

And building it exposed something worse, which is the real story. The first implementation merged by parse → apply → re-render, and parse → render turns out to be lossy: an ADR beginning ## Purpose instead of ## PURPOSE lost that entire section, preambles were dropped, out-of-order sections silently reordered. So the merge no longer rebuilds anything — it splices, locating the target heading's byte span and replacing only that, leaving every other byte untouched by construction.

Which means custom headings now work — ## DECISIONS is just a span to locate or a block to append. Your original use case is served after all, by a different route than either of us proposed.

A BEGIN IMMEDIATE also went into the store primitive rather than the call site, so the lost-update race is closed for all three writers of that row, not just this one.

Thank you for a genuinely strong first contribution. I would welcome your next.